-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Kotlin/Native implementation of BigDecimal and BigInteger #1200
base: kn-main
Are you sure you want to change the base?
Conversation
gradle.properties
Outdated
@@ -2,6 +2,7 @@ kotlin.code.style=official | |||
kotlin.incremental.js=true | |||
kotlin.incremental.multiplatform=true | |||
kotlin.mpp.stability.nowarn=true | |||
kotlin.native.binary.sourceInfoType=libbacktrace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to get better stack traces with this enabled? I remember you said it didn't help, so I'd rather leave it off if it's not working as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will remove. I eventually discovered that full stack traces are available in the build artifacts (e.g., build/reports/tests/linuxX64Test/index.html) just not in the console output.
|
||
actual override fun equals(other: Any?): Boolean = other is BigDecimal && delegate == other.delegate | ||
public actual override fun equals(other: Any?): Boolean = other is BigDecimal && delegate == other.delegate | ||
public actual override fun hashCode(): Int = 31 + delegate.hashCode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually 31 is multiplied in these hashCode implementations, is this intentional addition? Is there any reason we can't just take the delegate's hash code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I intentionally used addition instead of multiplication although either should work. I wanted to avoid using the unmodified hash code from the delegate because that would bin them the same if they were both added to a hashed container (e.g., Set
). That's unlikely but also trivial to avoid.
public actual operator fun plus(other: BigDecimal): BigDecimal = | ||
coalesceOrCreate(delegate + other.delegate, this, other) | ||
|
||
public actual operator fun minus(other: BigDecimal): BigDecimal = | ||
coalesceOrCreate(delegate - other.delegate, this, other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
public actual override fun toString(): String = toString(10) | ||
public actual fun toString(radix: Int): String = delegate.toString(radix) | ||
|
||
public actual override fun hashCode(): Int = 17 + delegate.hashCode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about adding 17, can we just take the delegate's hash code?
private companion object { | ||
/** | ||
* Returns a new or existing [BigDecimal] wrapper for the given delegate [value] | ||
* @param value The delegate value to wrap | ||
* @param left A candidate wrapper which may already contain [value] | ||
* @param right A candidate wrapper which may already contain [value] | ||
*/ | ||
fun coalesceOrCreate(value: IonSpinBigDecimal, left: BigDecimal, right: BigDecimal): BigDecimal = when (value) { | ||
left.delegate -> left | ||
right.delegate -> right | ||
else -> BigDecimal(value) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this coalesceOrCreate
might be a candidate for a generic function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be nice but our BigInteger
and BigDecimal
definitions don't have a common interface which defines val delegate
as a public member. I'd rather not add one either since the delegate is an internal implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing to add beyond what Matas said
Issue #
(none)
Description of changes
This change implements Kotlin/Native support for
BigInteger
/BigDecimal
by way of the IonSpin kotlin-multiplatform-bignum library. This change adds a workaround for building smithy-kotlin on Amazon Linux 2 because of linking issues with zlib (see runtime/build.gradle.kts).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.